-
Notifications
You must be signed in to change notification settings - Fork 68
[AL-2896] Video annotation serialization/deserialization using segment_index #635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| # Group segment by consecutive frames, since `segment_index` is not present | ||
| return cls._get_consecutive_frames( | ||
| sorted([annotation.frame for annotation in annotation_group])) | ||
| elif len(sorted_frame_segment_indices) == len(annotation_group): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question been a while since i have reviewed annotation_group, could you explain why this check is == len(annotation_group) ? Is it because we expect the number of segments to be the number of items in the annotation group?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! So this is just checking that every annotation inside annotation_group contains a segment_index.
segment_index must be present in every VideoObjectAnnotation or VideoClassificationAnnotation. If only some annotations contain segment_index, error is raised
| segment_groups = defaultdict(list) | ||
| for frame, segment_index in sorted_frame_segment_indices: | ||
| if segment_index < last_segment_id: | ||
| raise ValueError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question In what instances do we expect this error to occur, since we are fetching indices via enumerate and then sorting? Should we ever expect this to be a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be an issue if users construct their own Video annotations, and accidentally assign segment_indices in a non-ascending order
| def to_common(self, name: str, feature_schema_id: Cuid): | ||
| result = [] | ||
| for segment in self.segments: | ||
| for idx, segment in enumerate(self.segments): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment This is a really neat idea :)
| for group in segment_groups.values(): | ||
| frame_ranges.append((group[0], group[-1])) | ||
| return frame_ranges | ||
| else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question When would we expect only partial indices to occur?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If users are providing their own Video annotation objects!
Background
Currently, the only way to group video segments in SDK's
VideoObjectAnnotationsis to have an instance for every consecutive frame. This PR introduces a concept of segment_index, a new attribute in Video annotation classes, that can group annotation instances together into a video segment, without having to be consecutive frames.How does
segment_indexwork?segment_indexmust be present in everyVideoObjectAnnotationorVideoClassificationAnnotation. If only some annotations containsegment_index, error is raisedsegment_index, segments are evaluated using consecutive frames, same as the original logicsegment_indexwill be grouped within the same video segmentHeads up
segment_index. User can also serialize VideoObjectAnnotations withsegment_index, which will properly evaluate frames within the same segment. I updated the test cases to test thisVideoClassificationAnnotationsis not affected by this change much, because classification answer change usingVideoClassificationAnnotationsseems to not be supported. Will tackle this in another ticket